Improved macOS support: JNA #40
Conversation
…h Queue; File System Events)
…n FS event stream using the native macOS APIs
…sing the native APIs to the caller
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.0%) is below the target coverage (50.0%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## improved-macos-support-main #40 +/- ##
================================================================
- Coverage 80.5% 64.9% -15.6%
- Complexity 134 135 +1
================================================================
Files 16 20 +4
Lines 570 705 +135
Branches 66 80 +14
================================================================
- Hits 459 458 -1
- Misses 69 206 +137
+ Partials 42 41 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DavyLandman
left a comment
DavyLandman
left a comment
There was a problem hiding this comment.
Some small comments.
Impressive work.
| // Native memory (automatically deallocated when set to `null`) | ||
| private @Nullable FSEventStreamCallback callback; | ||
| private @Nullable Pointer stream; | ||
| private @Nullable Pointer queue; |
There was a problem hiding this comment.
I think they should be volatile, as their value will get ready by multiple threads, and you don't want thread-local caches. Since they could point towards bad pointers.
There was a problem hiding this comment.
All reads/writes to these fields are inside synchronized blocks. Comment added to clarify.
| private static final DispatchQueue DQ = DispatchQueue.INSTANCE; | ||
| private static final FileSystemEvents FSE = FileSystemEvents.INSTANCE; | ||
|
|
||
| // Native memory (automatically deallocated when set to `null`) |
There was a problem hiding this comment.
who takes care of that deallocation?
There was a problem hiding this comment.
The comment is a bit confusing. The main point it was supposed to convey is that the callback field is needed to keep a live reference to the callback (otherwise, it's prematurely GC'ed). Comment rephrased.
(To also answer the question: stream and queue are deallocated explicitly in method close() through invocations of FSE.FSEventStreamRelease and DO.dispatch_release. See also another comment below.)
| private volatile boolean closed; | ||
|
|
||
| public NativeEventStream(Path path, NativeEventHandler handler) throws IOException { | ||
| this.path = path.toRealPath(); // Resolve symbolic links |
There was a problem hiding this comment.
shouldn't we also store the original requested path? So that our events generated are in the context of the original path, and not the resolved path?
or otherwise, we should make it an argument requirement?
There was a problem hiding this comment.
All paths returned to consumers of the stream are relative to the root of the watch scope (so they can be used as contexts in WatchEvents). However, macOS always seems to be using real paths in events, so to be able to relativize, the real path of the root is needed.
For instance, the original path could be this (i.e., new TestDirectory().getTestDirectory()):
/var/folders/k3/fpm2wy9s6l16x1cchqr9k7wh0000gn/T/java-watch-test9831264884788018313
But, the paths in the incoming events are these:
/private/var/folders/k3/fpm2wy9s6l16x1cchqr9k7wh0000gn/T/java-watch-test9831264884788018313/a.txt
/private/var/folders/k3/fpm2wy9s6l16x1cchqr9k7wh0000gn/T/java-watch-test9831264884788018313/b.txt
/private/var/folders/k3/fpm2wy9s6l16x1cchqr9k7wh0000gn/T/java-watch-test9831264884788018313/d.txt
So, we cannot relativize to the original path. Instead, we need to compute path.toRealPath():
/private/var/folders/k3/fpm2wy9s6l16x1cchqr9k7wh0000gn/T/java-watch-test9831264884788018313
Then, we can relativize to path.toRealPath():
a.txt
b.txt
d.txt
| closed = false; | ||
| } | ||
|
|
||
| // Allocate native memory. (Checker Framework: The local variables are |
There was a problem hiding this comment.
are these CF comments needed? CF should understand assignment like this, it knows that createCallback does not return a nullable result?
There was a problem hiding this comment.
CF is a bit erratic about what it does, and doesn't, understand. (E.g., changing the order of the assignment to stream and queue confuses CF.) Anyway, I rewrote it a bit and removed the comment.
| public void callback(Pointer streamRef, Pointer clientCallBackInfo, | ||
| long numEvents, Pointer eventPaths, Pointer eventFlags, Pointer eventIds) { | ||
| // This function is called each time native events are issued by | ||
| // macOS. The purpose of this function is to perform the minimal |
There was a problem hiding this comment.
I might be wrong, but it reads like we should make sure that this function does not consume a lot of time? As it's a native callback?
If that is true, I think we should publish the event to a ConcurrentQueue, as we don't know how much work happens inside of the handler
There was a problem hiding this comment.
Alternative solution as discussed: make the class package private.
src/main/java/engineering/swat/watch/impl/mac/NativeEventStream.java
Outdated
Show resolved
Hide resolved
| DO.dispatch_release(queue); | ||
| } | ||
|
|
||
| // Deallocate native memory |
There was a problem hiding this comment.
doesn't this leave it to the GC/destroy to deallocate? isn't there an explicit function in the JNA framework we can call to cleanup?
There was a problem hiding this comment.
The comment was misplaced. The actual deallocation of the stream and the queue happens in FSE.FSEventStreamRelease and DO.dispatch_release on the lines above; the deallocation of the callback does happen by setting it to null, though (so it can then be GC'ed). Moved comment.
src/main/java/engineering/swat/watch/impl/mac/apis/FileSystemEvents.java
Show resolved
Hide resolved
2 participants
This PR adds the low-level JNA code to use native macOS APIs. See the commit messages for details.
Note: Only the functions of the APIs that we need are mapped -- not everything.